-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MSI installer #38
MSI installer #38
Conversation
…or Windows Server 2016. Fixes #34
// You can specify all the values or you can default the Build and Revision Numbers | ||
// by using the '*' as shown below: | ||
// [assembly: AssemblyVersion("1.0.*")] | ||
[assembly: AssemblyVersion("1.0.0.0")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not updated during the build, something to look into during the beta (should we set these to 0.0.0.1
for the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had ripped out the AssemblyInfo patching, but I might just add it back in.
@@ -0,0 +1,11 @@ | |||
<?xml version="1.0" encoding="utf-8" ?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use [CustomAction]
is this file still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove if I can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
installer/paket.dependencies
Outdated
|
||
source https://www.nuget.org/api/v2 | ||
|
||
nuget xunit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be cleaned up to only include the things needed for this project (e.g xunit and fluentassertions are not needed AFAIK)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some comments.
It also looks like this still has a dependency on Wix Toolset being installed, although it references the nuget package that contains all of the Wix binaries. I think the WixSharp Compiler.WixLocation
property needs to be set to the package location, similar to the Elasticsearch Windows installer:
…ompilation process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. Just left one small comment
[assembly: AssemblyCopyright("Copyright © 2017")] | ||
[assembly: AssemblyCompany("Elasticsearch B.V.")] | ||
[assembly: AssemblyProduct("Elasticsearch ODBC Installer")] | ||
[assembly: AssemblyCopyright("Copyright Elasticsearch B.V. ©")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this include the year?
No description provided.